-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Poc/split navs #49539
base: main
Are you sure you want to change the base?
Poc/split navs #49539
Conversation
This comment has been minimized.
This comment has been minimized.
trim.EDC2126F-EA24-40D4-87A4-E252B86DA87A.MOVThere are no page animations in the settings stack |
I took this for a quick spin on Chrome/web and it worked fairly well, including properly accounting for the browser back case. One thing I noticed, though I'm not sure if it's related to this PR or not: I occasionally experienced performance issues, where the responsiveness of the page would slow down and almost freeze up for a second or two. I saw this on the |
Definitely performance is something we need to keep in mind |
@mountiny If I understand correctly this is something that we want. Those screens have the bottom tab bar. Other bottom tab screens don't have animations from the beginning. @JmillsExpensify we are still working on adjusting performance tricks for these changes. Please point us to flows that feels slower than before so we can investigate it 🙇 |
Hmm, I dont think we want that, going deeper in the settings should have animations, same as going to a report from LHN has animation @Expensify/design @trjExpensify @JmillsExpensify |
@adamgrzybowski Wanted to report here as well that I think the performance on web for me is gradually getting worse and this morning it's noticeably slow, you can see it here as well which I have recorded for a different bug Screen.Recording.2024-09-25.at.10.12.04.mp4 |
Screen.Recording.2024-09-25.at.11.52.59.movBut doesn't that look weird? You can see how one bottom tab bar covers the previous one with animation. cc: @mountiny |
Yeah, I think that does look weird. The bottom tab bar feels like it's part of page being animated in, rather than persisting across the different pages. |
Yeah that looks a bit off. The view should go over the bottom navigation instead. Though there's another issue to put the bottom navigation on top in workspaces. Which should give us this: CleanShot.2024-09-26.at.11.06.21.mp4 |
d5a8d96
to
0283fa5
Compare
I agree that the bottom tab should not animate if it's staying on another page, too. I would defer to @dubielzyk-expensify and the design team to see how we want to handle this. |
The video that Jon shared is what I would expect and prefer. But if that's not technically feasible at the moment, I would rather have the bottom tabs than the animation. |
That's the problem. It may be tough to implement and if that's okay I would investigate this as follow-up. But let me explain why this is problematic at all. Previously, we had a navigator that displayed a bottom tab bar over every screen within itself. This approach did not allow us to place the bottom tab bar on every screen in the app. Only on screens in this particular navigator. And it definitely did not allow us to display the bottom tab bar conditionally, as in the "Workspaces screen" case. We decided to render the bottom on every screen that requires it instead. This approach gives us a lot of flexibility. We did know that we couldn't create an animation of sliding screen with the bottom tab onto another screen with the bottom tab. But we assumed that as in current use cases, we won't animate screens with the bottom tab so that's not a problem. Let's assume that we want to have this animation and we won't display it on every page but we will figure out a way to do it smarter. All screens that you can open from the menu visible in the video above are on the same level. Screen containing this menu is also on the same level. If that doesn't make sense, please consider the fact that we also need to handle the wide layout with this structure. That means they are rendered with one I am not saying it is impossible. I just want to give you context on how complex is adding this little animation. I have a few tricks in my mind but I would rather focus on this after migrating to the new structure. |
When it comes to performance. I started digging and couldn't reproduce the laggy RHP that you filmed @mountiny. I might have to try the high traffic account or something like that. However, I discovered a different issue when switching to the "Reports" tab. What you can notice in our POC is that you can't see the skeleton loading. The first render of the report screen is delayed in comparison to the main. before.mp4It doesn't look like a big delay but it feels laggy when you click it yourself. I investigate that and it looks like this doesn't happen on the main only by coincidence. I figured out a solution for that and tomorrow I will work on PR that we could merge to the main. It should help with the initial render of heavy screens in general and improve the user experience. Below you can see the test result after.mp4 |
I am fine exploring the animation later @adamgrzybowski, you should definitely test with a high-traffic account at least; you could even try the heavy performance testing account we provided to SWM. Testing with a fresh account is not representative of the users we care about the most. |
FYI: I played around with a solution I found yesterday regarding performance, and it may need more work and investigation. For now, I'll focus on other work left for this PR and get back to perf closer to review. |
…adjust getAdaptedState
…anups Poc/bottom tab cleanups
Hey, just a quick update from me. We have something that looks good. However, I must still clean the code and handle some edge cases. Screen.Recording.2024-12-12.at.16.34.34.mp4 |
Exciting, that looks great! |
Yeah , looking great |
Yeah that looks awesome! |
Details
Fixed Issues
$
PROPOSAL:
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop